Skip to content

[AIE2] Support for allowing direct VEXTRACT to 20-bit registers#233

Merged
abhinay-anubola merged 3 commits intoaie-publicfrom
sanubola.support.20bit.VEXTRACT
Feb 27, 2025
Merged

[AIE2] Support for allowing direct VEXTRACT to 20-bit registers#233
abhinay-anubola merged 3 commits intoaie-publicfrom
sanubola.support.20bit.VEXTRACT

Conversation

@abhinay-anubola
Copy link
Collaborator

@abhinay-anubola abhinay-anubola commented Nov 8, 2024 •
edited
Loading

  • This update introduces a new generic combiner that simplifies the sequence sext(trunc x) directly to x when applicable.
  • Added VExtract combiner that enables above generic combiner, thus we have 20-bit vextract.
  • The MachineVerifier has been updated to allow G_AIE_SEXT_EXTRACT_VECTOR_ELT and G_AIE_ZEXT_EXTRACT_VECTOR_ELT to accept 20-bit outputs.
  • Additionally, tests have been added and updated to reflect these functional changes.

andcarminati
andcarminati reviewed Nov 8, 2024
View reviewed changes
krishnamtibrewala
krishnamtibrewala reviewed Nov 8, 2024
View reviewed changes
krishnamtibrewala
krishnamtibrewala reviewed Nov 8, 2024
View reviewed changes
krishnamtibrewala
krishnamtibrewala reviewed Nov 8, 2024
View reviewed changes
krishnamtibrewala
krishnamtibrewala reviewed Nov 8, 2024
View reviewed changes
krishnamtibrewala
krishnamtibrewala reviewed Nov 8, 2024
View reviewed changes
krishnamtibrewala
krishnamtibrewala reviewed Nov 8, 2024
View reviewed changes
krishnamtibrewala
krishnamtibrewala reviewed Nov 8, 2024
View reviewed changes
krishnamtibrewala
krishnamtibrewala reviewed Nov 8, 2024
View reviewed changes
@krishnamtibrewala
Copy link
Collaborator

krishnamtibrewala commented Nov 8, 2024

Given that you mentioned there are no QoR gain, I would recommend you to re look at the instruction that consume S20 type reg.
Because for the optimization starts to trace back from an instruction that consumes S20 type which might not be captured in isNativeS20Consumer function.

@abhinay-anubola abhinay-anubola force-pushed the sanubola.support.20bit.VEXTRACT branch from 650d8a9 to d5d7cf0 Compare November 12, 2024 11:21
andcarminati
andcarminati reviewed Nov 12, 2024
View reviewed changes
gbossu
gbossu reviewed Nov 13, 2024
View reviewed changes
gbossu
gbossu reviewed Nov 13, 2024
View reviewed changes
@abhinay-anubola abhinay-anubola force-pushed the sanubola.support.20bit.VEXTRACT branch from d5d7cf0 to f12f1a4 Compare November 14, 2024 09:13
gbossu
gbossu reviewed Nov 14, 2024
View reviewed changes
gbossu
gbossu reviewed Nov 15, 2024
View reviewed changes
krishnamtibrewala
krishnamtibrewala reviewed Nov 18, 2024
View reviewed changes
andcarminati
andcarminati reviewed Dec 16, 2024
View reviewed changes
andcarminati
andcarminati reviewed Dec 16, 2024
View reviewed changes
@abhinay-anubola abhinay-anubola force-pushed the sanubola.support.20bit.VEXTRACT branch from 35027af to 000b09e Compare December 17, 2024 05:58
andcarminati
andcarminati reviewed Dec 17, 2024
View reviewed changes
andcarminati
andcarminati reviewed Dec 17, 2024
View reviewed changes
@abhinay-anubola abhinay-anubola force-pushed the sanubola.support.20bit.VEXTRACT branch from 000b09e to e7731e6 Compare December 17, 2024 09:08
gbossu
gbossu reviewed Dec 30, 2024
View reviewed changes
llvm/test/CodeGen/RISCV/GlobalISel/jumptable.ll Outdated
Copy link
Collaborator

@gbossu gbossu Dec 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Did you check all targets?

Copy link
Collaborator Author

@abhinay-anubola abhinay-anubola Jan 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I have checked all targets.

gbossu
gbossu reviewed Dec 30, 2024
View reviewed changes
llvm/lib/Target/AIE/AIE2PreLegalizerCombiner.cpp Outdated
Copy link
Collaborator

@gbossu gbossu Dec 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to change the return types? I would expect that we only need to add a %10:_(s32) = G_ASSERT_[S/Z]EXT %9, 16 and keep the rest intact thanks to the new sext(trunc x) combiner you added previously.

Copy link
Collaborator Author

@abhinay-anubola abhinay-anubola Jan 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we need to change the return types, because the pattern that is written in new sext(trunc x) combiner will not match in this case as m_SpecificType is trying to match s20 but return type here is s32.
mi_match(SrcReg, MRI, m_GTrunc(m_all_of(m_Reg(Reg), m_SpecificType(DstTy))))

@abhinay-anubola abhinay-anubola force-pushed the sanubola.support.20bit.VEXTRACT branch from e7731e6 to 467672d Compare January 2, 2025 10:54
andcarminati
andcarminati previously approved these changes Jan 2, 2025
View reviewed changes
Copy link
Collaborator

@andcarminati andcarminati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@abhinay-anubola abhinay-anubola force-pushed the sanubola.support.20bit.VEXTRACT branch from 2d5aaad to de8468e Compare January 8, 2025 10:42
@abhinay-anubola abhinay-anubola force-pushed the sanubola.support.20bit.VEXTRACT branch from de8468e to c3cba3e Compare February 20, 2025 09:52
konstantinschwarz
konstantinschwarz reviewed Feb 20, 2025
View reviewed changes
llvm/test/CodeGen/AMDGPU/GlobalISel/combine-sext-trunc.mir Outdated
Copy link
Collaborator

@konstantinschwarz konstantinschwarz Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I guess we could still eliminate the G_TRUNC, and change G_SEXT to extend from 32 -> 64?

Copy link
Collaborator Author

@abhinay-anubola abhinay-anubola Feb 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This story requires a (sext (trunc x)) → x combiner; we can create a new ticket for the G_TRUNC elimination

Copy link
Collaborator

@konstantinschwarz konstantinschwarz Feb 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed 👍

@abhinay-anubola abhinay-anubola force-pushed the sanubola.support.20bit.VEXTRACT branch from c3cba3e to fcf4285 Compare February 24, 2025 10:04
@abhinay-anubola abhinay-anubola changed the title Support for allowing direct VEXTRACT to 20-bit registers [AIE2] Support for allowing direct VEXTRACT to 20-bit registers Feb 24, 2025
@abhinay-anubola abhinay-anubola force-pushed the sanubola.support.20bit.VEXTRACT branch from fcf4285 to 14eea26 Compare February 24, 2025 10:25
niwinanto
niwinanto reviewed Feb 25, 2025
View reviewed changes
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp

bool CombinerHelper::matchCombineSextTrunc(MachineInstr &MI, Register &Reg) {
assert(MI.getOpcode() == TargetOpcode::G_SEXT && "Expected a G_SEXT");
const Register DstReg = MI.getOperand(0).getReg();
Copy link
Collaborator

@niwinanto niwinanto Feb 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: you could use const auto [DstReg, DstTy, SrcReg, SrcTy] = MI.getFirst2RegLLTs()

konstantinschwarz
konstantinschwarz approved these changes Feb 27, 2025
View reviewed changes
llvm/test/CodeGen/AMDGPU/GlobalISel/combine-sext-trunc.mir Outdated
Copy link
Collaborator

@konstantinschwarz konstantinschwarz Feb 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed 👍

@abhinay-anubola abhinay-anubola merged commit 9a67e50 into aie-public Feb 27, 2025
6 checks passed
@abhinay-anubola abhinay-anubola deleted the sanubola.support.20bit.VEXTRACT branch February 27, 2025 05:54
mgehre-amd pushed a commit that referenced this pull request Aug 21, 2025
@josel-amd
Merge pull request #233 from Xilinx/jose.post_pdll_xilinx_3
9762135 
Bump to March 7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@niwinanto niwinanto niwinanto left review comments

@gbossu gbossu gbossu left review comments

@krishnamtibrewala krishnamtibrewala krishnamtibrewala left review comments

@konstantinschwarz konstantinschwarz konstantinschwarz approved these changes

@andcarminati andcarminati andcarminati left review comments

@abnikant abnikant Awaiting requested review from abnikant abnikant is a code owner

@khallouh khallouh Awaiting requested review from khallouh khallouh is a code owner

@martien-de-jong martien-de-jong Awaiting requested review from martien-de-jong martien-de-jong is a code owner

@SagarMaheshwari99 SagarMaheshwari99 Awaiting requested review from SagarMaheshwari99 SagarMaheshwari99 is a code owner

@stephenneuendorffer stephenneuendorffer Awaiting requested review from stephenneuendorffer stephenneuendorffer is a code owner

@F-Stuckmann F-Stuckmann Awaiting requested review from F-Stuckmann F-Stuckmann is a code owner

@katerynamuts katerynamuts Awaiting requested review from katerynamuts katerynamuts is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@abhinay-anubola @krishnamtibrewala @niwinanto @andcarminati @gbossu @konstantinschwarz